You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
At first I noticed a crash when running the tests of this project. It was due to weird chars in my CWD.
This lead me to discover a bug where the default value description of a URL checks for the absoluteString value of itself instead of the path + whether it is a file URL!
Note: I did not add a test specifically, I don’t really know how this would be tested. Maybe create a folder with a weird name and check everything is good? Also a test with an https URL as default w/ a path being the same as current CWD?
Tell me, I’ll do them if needed.
Checklist
I've added at least one test that validates that my change is working, if appropriate
I've followed the code style of the rest of the project
After a quick skim of the documentation, it looks like this might actually be a Foundation bug. FileManager.currentDirectoryPath is defined as returning a type String, but it appears possible it will return nil. It should probably be defined as returning String?.
@kylemacomber I don’t think so. Maybe it’s a separate issue, but unrelated to the one I reported.
URL(string: FileManager.default.currentDirectoryPath) is likely to fail, and is definitely not supposed to be used to create a file URL.
It might not return nil if by a lucky chance the current directory path happens to be a valid URL (e.g. a path with no space in it), but 1/ the returned URL will never be a file URL, and 2/ if the current path contains a space or any character not allowed in a URL the init will definitely return nil.
The correct way to init a file URL is URL(fileURLWithString: "/path/to whatever"), which will never return nil in addition to correctly returning a file URL.
EDIT: And of course, absoluteString is not the path of a URL. For a file URL it could yield something like file:///path/to%20whatever for instance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At first I noticed a crash when running the tests of this project. It was due to weird chars in my CWD.
This lead me to discover a bug where the default value description of a URL checks for the
absoluteStringvalue of itself instead of the path + whether it is a file URL!Note: I did not add a test specifically, I don’t really know how this would be tested. Maybe create a folder with a weird name and check everything is good? Also a test with an https URL as default w/ a path being the same as current CWD?
Tell me, I’ll do them if needed.
Checklist